Skip to content

Fix signature matching for externals when abstract alias hides function arity #8045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
cknitt merged 1 commit into from
Dec 6, 2025
Merged

Fix signature matching for externals when abstract alias hides function arity #8045

cknitt merged 1 commit into from
Dec 6, 2025

Conversation

@cknitt
Copy link
Member

@cknitt cknitt commented Nov 30, 2025

Fixes #8038

@cknitt cknitt requested a review from cristianoc November 30, 2025 10:08
@cknitt
Copy link
Member Author

cknitt commented Nov 30, 2025

@cristianoc fix by Codex - could you have a look?

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 30, 2025 •
edited
Loading

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@8045

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@8045

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@8045

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@8045

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@8045

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@8045

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@8045

commit: ceedc3c

@cknitt cknitt force-pushed the fix-opaque-externals branch from 36611cd to 3a78abe Compare November 30, 2025 10:16
cristianoc
cristianoc reviewed Dec 2, 2025
View reviewed changes
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this copied a certain style from existing tests lol.
Opaque test name.

And "regression".
If it is a regression, point out in what compiler version the issue was introduced.

cristianoc
cristianoc requested changes Dec 5, 2025
View reviewed changes
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking to clean up: proper name for the test file, and description in the PR title of what is being fixed.
That said, the question is whether it can be fixed early by trying to avoid to create the issue in the first place.

@cknitt
Copy link
Member Author

cknitt commented Dec 5, 2025 •
edited
Loading

@cristianoc

If it is a regression, point out in what compiler version the issue was introduced.

It worked fine in ReScript 11, but broke in ReScript 12.

proper name for the test file

Can you clarify? I guess the "gpr_<issue_number>" prefix following the conventions from other files is fine?

opaque is not totally wrong as it is about an opaque type in an interface being implemented as a function type in the implementation.
external is not totally wrong either as the problem does not occur without the external declaration.

What would you suggest as the file name / PR title?

@cristianoc
Copy link
Collaborator

cristianoc commented Dec 5, 2025

@cristianoc

If it is a regression, point out in what compiler version the issue was introduced.

It worked fine in ReScript 11, but broke in ReScript 12.

proper name for the test file

Can you clarify? I guess the "gpr_<issue_number>" prefix following the conventions from other files is fine?

opaque is not totally wrong as it is about an opaque type in an interface being implemented as a function type in the implementation. external is not totally wrong either as the problem does not occur without the external declaration.

What would you suggest as the file name / PR title?

That's some old leftover convention that creates an unnecessary indirection and says nothing about what's tested.

cristianoc
cristianoc approved these changes Dec 5, 2025
View reviewed changes
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to go after the suggestions are in.
Still I would ask the agent if this can be prevented earlier instead of fixing just before signature inclusion.

@cknitt cknitt changed the title Fix external signature inclusion for opaque function types Fix signature matching for externals when abstract alias hides function arity Dec 6, 2025
@cknitt cknitt force-pushed the fix-opaque-externals branch from 3a78abe to ceedc3c Compare December 6, 2025 09:41
@cknitt
Copy link
Member Author

cknitt commented Dec 6, 2025

@cristianoc Was not successful in preventing earlier instead of fixing just before signature inclusion.

I did change the naming though, please have another look.

@cristianoc
Copy link
Collaborator

cristianoc commented Dec 6, 2025

@cristianoc Was not successful in preventing earlier instead of fixing just before signature inclusion.

I did change the naming though, please have another look.

Looks great.
If it's not easy to prevent then not worth sweating it.
The current fix does the job.

@cknitt cknitt merged commit 394848f into rescript-lang:master Dec 6, 2025
25 checks passed
@cknitt cknitt deleted the fix-opaque-externals branch December 6, 2025 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cristianoc cristianoc cristianoc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Signature Mismatch on external using opaque function type in rescript v12

2 participants

@cknitt @cristianoc